Skip to content

Use safe_malloc instead of new when creating new_envoy_map_entry#2632

Merged
RyanTheOptimist merged 4 commits intoenvoyproxy:mainfrom
RyanTheOptimist:new_envoy_map_entry
Oct 31, 2022
Merged

Use safe_malloc instead of new when creating new_envoy_map_entry#2632
RyanTheOptimist merged 4 commits intoenvoyproxy:mainfrom
RyanTheOptimist:new_envoy_map_entry

Conversation

@RyanTheOptimist
Copy link
Contributor

Use safe_malloc instead of new when creating new_envoy_map_entry
since release_envoy_map uses free instead of delete. Found when
building this code internally with some sort of strict checking enabled

Signed-off-by: Ryan Hamilton rch@google.com

Risk Level: Low
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A

…e release_envoy_map uses free instead of delete

Signed-off-by: Ryan Hamilton <rch@google.com>
…e release_envoy_map uses free instead of delete

Signed-off-by: Ryan Hamilton <rch@google.com>
@RyanTheOptimist
Copy link
Contributor Author

/assign @snowp

@RyanTheOptimist
Copy link
Contributor Author

@snowp I sent this your way based on the Obj-C/C/C++ discussion we had on Slack. Feel free to redirect though!

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice this makes sense to me, one suggestion

Is this check something we can enable for the EM build? Or is this some internal secret sauce?

Comment on lines +21 to +22
envoy_map_entry* header_array =
static_cast<envoy_map_entry*>(safe_malloc(sizeof(envoy_map_entry) * 0));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use makeEnvoyMap here? Would be nice to keep the init logic in one place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I was able to do this for both this test and the subsequent one. The rest of the tests, though, seem more elaborate. They set up envoy_map instances in which the envoy_data is wired up to use envoy_test_release() when releasing the data to do tracking. makeEnvoyMap seems to use copyToBridgeData() to populate the map which I think makes this a non-starter. But I'm not entirely familiar with this code so maybe there's a cleaner approach?

Signed-off-by: Ryan Hamilton <rch@google.com>
@RyanTheOptimist
Copy link
Contributor Author

Nice this makes sense to me, one suggestion

Is this check something we can enable for the EM build? Or is this some internal secret sauce?

Interesting question. It does look like an internal secret sauce thing in tcmalloc, according to some internal docs. However, the chromium version of tcmalloco has something basically similar. There error comes from here:

https://chromium.googlesource.com/chromium/src/third_party/tcmalloc/chromium/+/refs/heads/main/src/debugallocation.cc#474

So it might be something to investigate, but i don't think it works out of the box.

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jpsim
Copy link
Contributor

jpsim commented Oct 26, 2022

/retest

Signed-off-by: Ryan Hamilton <rch@google.com>
@RyanTheOptimist RyanTheOptimist merged commit 8c4814b into envoyproxy:main Oct 31, 2022
jpsim added a commit that referenced this pull request Nov 4, 2022
…builder-function

* origin/main:
  remove the use of deprecated flag (#2658)
  Cronvoy: Map EM Errors to Cronet API Errors II (#1594) (#2633)
  build: revert boring patch (#2651)
  Bump Lyft Support Rotation (#2654)
  fix one issue blocking bumping Envoy (#2649)
  ci: remove Snow from Lyft EM rotation (#2650)
  ci: increasing timeouts (#2653)
  python: Apply Envoy python-yapf formatting (#2648)
  repo: Shellcheck cleanups (#2646)
  repo: Switch `pip_install` -> `pip_parse` (#2647)
  Use safe_malloc instead of new when creating new_envoy_map_entry (#2632)
  python: Pin requirement hashes (#2643)
  Disable flaky TestConfig.StringAccessors (#2642)
  ci: migrate from set-output to GITHUB_OUTPUT (#2625)
  Add a comment to addPlatformFilter (#2634)
  Allow Cronvoy to build with proguard. (#2635)
  Update Envoy (#2630)
  Add support for Platform and Native filters to C++ EngineBuilder (#2626)
  Register getaddrinfo in extension_registry (#2627)
  dns: stop using cares DNS resolver (#2618)

Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants